Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to export Vault Token #127

Merged
merged 4 commits into from
Oct 1, 2020
Merged

Conversation

fean5959a
Copy link
Contributor

Usefull when run terraform, packer etc ... in a workflow and want to use directly Vault token.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 22, 2020

CLA assistant check
All committers have signed the CLA.

@jasonodonnell jasonodonnell self-requested a review September 22, 2020 18:03
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A number of tests broke (you can ignore e2e-tls, we have a bug where community members can't access our GH secrets set on the repo). From what I gathered through the action logs, it appears exportToken is exporting the token even though it defaults to false.

I wonder if you'll need better equality checking similar to tls-skip-verify: https://github.com/hashicorp/vault-action/blob/master/src/action.js#L31-L34

To run the basic test you can run the following in the root of the project:
npm run test.

Additionally, if there is a bug and it's fixed, we'll want test coverage on this to ensure the functionality of this feature.

@fean5959a
Copy link
Contributor Author

Your right, my condition was wrong.
Now all tests pass !

@jasonodonnell
Copy link
Contributor

Hi @fean5959a, thanks for fixing that! We'll also need additional test coverage to ensure this configurable works. Do you think you can add it? I think the basic test (src/action.test.js) should be fine.

@fean5959a
Copy link
Contributor Author

Yes I can, and I encountered some bug when secret has a form of my-secret-key (with dash) ...
I fixed it and update your tests with these value (i.e otherSecret > "other-Secret-dash"), using "" to tel it this is a string.
I don't commited this code, do want I make an other PR or in the same ?

dist/index.js Outdated Show resolved Hide resolved
Fix key with dash
@fean5959a
Copy link
Contributor Author

Do you think my PR can be merged ? accepted or are you checking it ?

@jasonodonnell jasonodonnell self-requested a review September 29, 2020 13:03
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fean5959a Everything looks good and I'd like to merge this, but have one more request. Can you remove the changes to dist/index.js? GitHub Actions uses this file when someone invokes the action and we've found that a lot of folks using the action point to master instead of tagged releases. Due to this we're cautious about updating this file and do it when we officially release Vault Action (2nd Tuesday of each month) to ensure we properly test all merged PRs.

@jasonodonnell jasonodonnell self-requested a review October 1, 2020 14:21
@jasonodonnell jasonodonnell merged commit 2f76ad3 into hashicorp:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants